-
Notifications
You must be signed in to change notification settings - Fork 18
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix chain elements init function #506
Fix chain elements init function #506
Conversation
f3cddab
to
fd36f99
Compare
pkg/networkservice/up/server.go
Outdated
u.initErr = initFunc(ctx, u.vppConn) | ||
}) | ||
return u.initErr | ||
if !u.inited.Load() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks not thread-safe. A few goroutines could read u.inited
as a false and block on the u.initMutex.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably we could add something like this https://go.dev/play/p/f2J7aioP59W and add this into sdk/tools as a common solution for chain elements with init functions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@edwarnicke Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As an alternative, we also could use a personal Executor.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@denis-tingaikin
I like the idea about extended sync.Once and add it to sdk/tools.
Prepared PR: networkservicemesh/sdk#1224
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
made it inline
Signed-off-by: Artem Glazychev <[email protected]>
fd36f99
to
600ec63
Compare
This problem is actual. So merging. @edwarnicke Please have a look at networkservicemesh/sdk#1224 this can a bit simplify these changes. |
…k-vpp@main PR link: networkservicemesh/sdk-vpp#506 Commit: 807bc2b Author: Artem Glazychev Date: 2022-02-04 17:12:04 +0700 Message: - Fix chain elements init function (#506) Signed-off-by: NSMBot <[email protected]>
…k-vpp@main PR link: networkservicemesh/sdk-vpp#506 Commit: 807bc2b Author: Artem Glazychev Date: 2022-02-04 17:12:04 +0700 Message: - Fix chain elements init function (#506) Signed-off-by: NSMBot <[email protected]>
…k-vpp@main PR link: networkservicemesh/sdk-vpp#506 Commit: 807bc2b Author: Artem Glazychev Date: 2022-02-04 17:12:04 +0700 Message: - Fix chain elements init function (#506) Signed-off-by: NSMBot <[email protected]>
…k-vpp@main PR link: networkservicemesh/sdk-vpp#506 Commit: 807bc2b Author: Artem Glazychev Date: 2022-02-04 17:12:04 +0700 Message: - Fix chain elements init function (#506) Signed-off-by: NSMBot <[email protected]>
…k-vpp@main PR link: networkservicemesh/sdk-vpp#506 Commit: 807bc2b Author: Artem Glazychev Date: 2022-02-04 17:12:04 +0700 Message: - Fix chain elements init function (#506) Signed-off-by: NSMBot <[email protected]>
Closes: networkservicemesh/deployments-k8s#2800
Motivation
If context was canceled (e.g by timeout) in the middle of the chain, and the next chain element has
init
function withsync.Once
- we have no chance to callinit
again. We store the error and all new Requests will fail with that error.Solution
Call the
init
function only the first time, OR if the first call returned an error.Signed-off-by: Artem Glazychev [email protected]